Skip to content

Allow to customize legend and graph style in TrendingTask #2591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
knopers8 merged 20 commits into from
Aug 14, 2025
Merged

Allow to customize legend and graph style in TrendingTask #2591

knopers8 merged 20 commits into from
Aug 14, 2025

Conversation

@mvishiu11
Copy link
Contributor

@mvishiu11 mvishiu11 commented Aug 2, 2025

Added the following features to allow for better trending customization:

  1. Allowed for changing legend position by passing (x1, y1, x2, y2) coordinates as well as nColumns parameter
  2. Allowed for more granular modification of graph style by passing a list of parameters to style field. TLDR: lineColor (parameter here is an integer precisely as defined by TColor class), markerColor (as above), markerStyle (integer as defined by TAttMarker class), lineWidth (integer, self-explanatory).

Also added an example workflow which uses the new features for FT0 trending (below the snippets within plots option):

"plots": [
          {
            "name": "trend_cycle_duration_ntf_corr",
            "title": "cycle duration: ns/TF;time;cycle duration [ns/TimeFrames]",
            "legend": { "enabled": true, "x1": 0.70, "y1": 0.70, "x2": 0.93, "y2": 0.90, "nColumns": 1 },
            "graphs": [
              {
                "title": "cycle duration [ns]",
                "varexp": "CycleDuration.entries:time",
                "selection": "",
                "option": "*LP",
                "style": { "lineColor": 38, "markerColor": 4, "markerStyle": 20, "lineWidth": 2 }
              },
              {
                "title": "cycle duration [TimeFrames]",
                "varexp": "CycleDurationNTF.entries:time",
                "selection": "",
                "option": "*LP",
                "style": { "lineColor": 8, "markerColor": 3, "markerStyle": 30, "lineWidth": 2 }
              },
              {
                "title": "cycle duration: ns/TF;time;cycle duration [ns/TimeFrames]",
                "varexp": "CycleDuration.entries/CycleDurationNTF.entries:time",
                "selection": "",
                "option": "*LP",
                "style": { "lineColor": 31, "markerColor": 30, "markerStyle": 23, "lineWidth": 2 }
              }
            ]
          }
]

knopers8
knopers8 requested changes Aug 4, 2025
View reviewed changes
Copy link
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution.

Please have a look at my suggestions and questions. I am quite surprised by the amount of removed/rephrased comments and refactored code which is not related to the PR. This obfuscated the PR significantly and made the review unnecessarily cumbersome. I strongly advise you to review each changed snippet separately before including it in a commit, git add -p allows you to do that.

@knopers8
Copy link
Collaborator

knopers8 commented Aug 7, 2025

BTW, please also document the new features at https://github.com/AliceO2Group/QualityControl/blob/master/doc/PostProcessing.md#the-trendingtask-class

justonedev1
justonedev1 reviewed Aug 7, 2025
View reviewed changes
@justonedev1
Copy link
Collaborator

justonedev1 commented Aug 7, 2025

@mvishiu11 I have another question. Is there a reason why you are rewording log messages as a part of this PR? It has nothing to do with the features you are implementing and it is making your PR quite polluted and hard to navigate. If you don't agree with the wording create separate PR for that.

@mvishiu11
Copy link
Contributor Author

mvishiu11 commented Aug 7, 2025

Based on comments from @knopers8 and @justonedev1 I have realized that some of my changes related to debugging were left in the code when I created the PR. I did my best to find and correct such cases. Please let me know if you find places where I have missed something. Thank you and sorry for the confusion 🙏

@mvishiu11
Copy link
Contributor Author

mvishiu11 commented Aug 7, 2025

BTW, please also document the new features at https://github.com/AliceO2Group/QualityControl/blob/master/doc/PostProcessing.md#the-trendingtask-class

Documentation added in [75ea283](75ea283)

@mvishiu11
Copy link
Contributor Author

mvishiu11 commented Aug 7, 2025 •
edited
Loading

@mvishiu11 I have another question. Is there a reason why you are rewording log messages as a part of this PR? It has nothing to do with the features you are implementing and it is making your PR quite polluted and hard to navigate. If you don't agree with the wording create separate PR for that.

I rolled back all the changes to ILOGs in 2f42fbc. Please let me know if I missed some, but it should be all of them

@knopers8
Copy link
Collaborator

knopers8 commented Aug 7, 2025

The CI checks are not yet enabled because you are a first-time contributor. They will start once I approve the PR. This being said, I can already see that clang-format will fail. Please see https://github.com/AliceO2Group/CodingGuidelines#formatting-tool to know how to format your code accordingly.

knopers8
knopers8 reviewed Aug 7, 2025
View reviewed changes
knopers8
knopers8 reviewed Aug 7, 2025
View reviewed changes
knopers8
knopers8 reviewed Aug 7, 2025
View reviewed changes
knopers8
knopers8 reviewed Aug 7, 2025
View reviewed changes
knopers8
knopers8 reviewed Aug 7, 2025
View reviewed changes
@knopers8
Copy link
Collaborator

knopers8 commented Aug 7, 2025

Based on comments from @knopers8 and @justonedev1 I have realized that some of my changes related to debugging were left in the code when I created the PR. I did my best to find and correct such cases. Please let me know if you find places where I have missed something. Thank you and sorry for the confusion 🙏

Please bear in mind that each change you propose has to be read and validated by a reviewer. If a change does not concern the goal of a PR, we can only guess the intention. Imagine commits with no messages and how much more time you would need to guess what the commit attempts to achieve. In your PR, there was a lot of changes with unclear intention.

I assume that such a review process and some tools are new to you, so I will hold no grudge, but next time, please review the proposed changes yourself first before asking for someone else's time.

knopers8
knopers8 reviewed Aug 7, 2025
View reviewed changes
@mvishiu11
Copy link
Contributor Author

mvishiu11 commented Aug 7, 2025 •
edited
Loading

Based on comments from @knopers8 and @justonedev1 I have realized that some of my changes related to debugging were left in the code when I created the PR. I did my best to find and correct such cases. Please let me know if you find places where I have missed something. Thank you and sorry for the confusion 🙏

Please bear in mind that each change you propose has to be read and validated by a reviewer. If a change does not concern the goal of a PR, we can only guess the intention. Imagine commits with no messages and how much more time you would need to guess what the commit attempts to achieve. In your PR, there was a lot of changes with unclear intention.

I assume that such a review process and some tools are new to you, so I will hold no grudge, but next time, please review the proposed changes yourself first before asking for someone else's time.

Of course, thank for your patience. I will try to make PRs smaller and more focused and review the changes thoroughly next time.

@mvishiu11 mvishiu11 requested a review from knopers8 August 8, 2025 11:07
knopers8
knopers8 reviewed Aug 8, 2025
View reviewed changes
knopers8
knopers8 reviewed Aug 8, 2025
View reviewed changes
knopers8
knopers8 reviewed Aug 8, 2025
View reviewed changes
@mvishiu11 mvishiu11 requested a review from knopers8 August 8, 2025 13:06
knopers8
knopers8 reviewed Aug 11, 2025
View reviewed changes
@mvishiu11 mvishiu11 requested a review from knopers8 August 12, 2025 07:54
knopers8
knopers8 requested changes Aug 13, 2025
View reviewed changes
Copy link
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I compiled your PR, and played with the settings to confirm everything is fine. I discovered some minor issues with the documentation, otherwise, it behaves as expected. Once my last batch of comments is addressed, I don't expect to have anything more and the PR will be good to go in. Thanks a lot.

@knopers8
Copy link
Collaborator

knopers8 commented Aug 13, 2025

@afurs , @andreasmolander are you good with the changes to DigitQcTask?

@mvishiu11 mvishiu11 requested a review from knopers8 August 13, 2025 11:56
@andreasmolander
Copy link
Collaborator

andreasmolander commented Aug 13, 2025 •
edited
Loading

@afurs , @andreasmolander are you good with the changes to DigitQcTask?

Maybe just quickly remove them for clarity, I understood they were included by mistake @mvishiu11

@mvishiu11
Copy link
Contributor Author

mvishiu11 commented Aug 13, 2025

@afurs , @andreasmolander are you good with the changes to DigitQcTask?

Maybe just quickly remove them for clarity, I understood they were included by mistake @mvishiu11

Removed in bcadfa3

@knopers8 knopers8 changed the title feat/better trending customization Allow to customize legend and graph style in TrendingTask Aug 13, 2025
knopers8
knopers8 approved these changes Aug 13, 2025
View reviewed changes
Copy link
Collaborator

@knopers8 knopers8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@knopers8 knopers8 enabled auto-merge (squash) August 13, 2025 12:08
@knopers8 knopers8 disabled auto-merge August 14, 2025 07:00
@knopers8 knopers8 merged commit 3dff0d1 into AliceO2Group:master Aug 14, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knopers8 knopers8 knopers8 approved these changes

@andreasmolander andreasmolander Awaiting requested review from andreasmolander andreasmolander is a code owner

@afurs afurs Awaiting requested review from afurs afurs is a code owner

@Barthelemy Barthelemy Awaiting requested review from Barthelemy Barthelemy is a code owner

@justonedev1 justonedev1 Awaiting requested review from justonedev1 justonedev1 is a code owner

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mvishiu11 @knopers8 @justonedev1 @andreasmolander